Skip to content

Admin changes#3737

Open
Ulincsys wants to merge 22 commits intomainfrom
admin-changes
Open

Admin changes#3737
Ulincsys wants to merge 22 commits intomainfrom
admin-changes

Conversation

@Ulincsys
Copy link
Copy Markdown
Collaborator

Description

  • Add admin dashboard
  • Add admin API routes
  • Add admin_required decorator to protect admin routes

Notes for Reviewers

  • There's a lot more work to do on this interface, but I'd like to get it into main so it stays up to date instead of lagging behind again.
  • Admin users cannot be created via the frontend, and must be added via the augur user add command with the --admin flag

Signed commits

  • Yes, I signed my commits.

Ulincsys and others added 22 commits March 20, 2023 18:39
Add @requires_admin decorator
Add clarity to backend start error status

Signed-off-by: Ulincsys <ulincsys@gmail.com>
- Add ssl decorator to config endpoints
- Fix syntax error in admin_required decorator
- Update dashboard endpoint to use config class directly
- Update dashboard styles with more consistent colors
- Implement config update functionality in admin dashboard

Signed-off-by: Ulincsys <28362836a@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
@Ulincsys Ulincsys requested a review from sgoggins as a code owner February 25, 2026 01:17
if signal.getsignal(signal.SIGHUP) != signal.SIG_DFL:
cmd = "nohup " + cmd

Popen(cmd, shell=True)

Check failure

Code scanning / Bandit

subprocess call with shell=True identified, security issue. Error

subprocess call with shell=True identified, security issue.
from ..server import app
import sqlalchemy as s
import json
from subprocess import run, PIPE, Popen

Check notice

Code scanning / Bandit

Consider possible security implications associated with the subprocess module. Note

Consider possible security implications associated with the subprocess module.
@app.route(f"/{AUGUR_API_VERSION}/admin/shutdown")
@admin_required
def shutdown_system():
run("augur backend stop-collection-blocking".split(), stdin=PIPE, stdout=PIPE, stderr=PIPE)

Check notice

Code scanning / Bandit

subprocess call - check for execution of untrusted input. Note

subprocess call - check for execution of untrusted input.
@admin_required
def shutdown_system():
run("augur backend stop-collection-blocking".split(), stdin=PIPE, stdout=PIPE, stderr=PIPE)
Popen("augur backend stop", shell=True, stdin=PIPE, stdout=PIPE, stderr=PIPE)

Check notice

Code scanning / Bandit

Starting a process with a partial executable path Note

Starting a process with a partial executable path
@admin_required
def shutdown_system():
run("augur backend stop-collection-blocking".split(), stdin=PIPE, stdout=PIPE, stderr=PIPE)
Popen("augur backend stop", shell=True, stdin=PIPE, stdout=PIPE, stderr=PIPE)

Check notice

Code scanning / Bandit

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
@@ -0,0 +1,29 @@
from subprocess import run, PIPE, Popen

Check notice

Code scanning / Bandit

Consider possible security implications associated with the subprocess module. Note

Consider possible security implications associated with the subprocess module.
# Ignore SIGTERM from parent process (since we're terminating our parent)
signal.signal(signal.SIGTERM, lambda signum, frame: None)

run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE)

Check notice

Code scanning / Bandit

Starting a process with a partial executable path Note

Starting a process with a partial executable path
# Ignore SIGTERM from parent process (since we're terminating our parent)
signal.signal(signal.SIGTERM, lambda signum, frame: None)

run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE)

Check notice

Code scanning / Bandit

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
signal.signal(signal.SIGTERM, lambda signum, frame: None)

run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE)
Popen("augur backend stop", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE).wait()

Check notice

Code scanning / Bandit

Starting a process with a partial executable path Note

Starting a process with a partial executable path
signal.signal(signal.SIGTERM, lambda signum, frame: None)

run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE)
Popen("augur backend stop", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE).wait()

Check notice

Code scanning / Bandit

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
import beaker

from flask import request, jsonify, current_app
from flask import request, jsonify, current_app, abort
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W0611: Unused current_app imported from flask (unused-import)

@@ -38,6 +40,13 @@ def unsupported_method(error):

return render_message("405 - Method not supported", "The resource you are trying to access does not support the request method used"), 405
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)

if AUGUR_API_VERSION in str(request.url_rule):
return jsonify({"status": "Forbidden"}), 403

return render_message("403 - Forbidden", "You do not have permission to view this page"), 403
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)

raise e

return render_message("500 - Internal Server Error", "An error occurred while trying to service your request. Please try again, and if the issue persists, please file a GitHub issue with the below error message:", error=stacktrace), 500
return render_message("500 - Internal Server Error", """An error occurred while trying to service your request.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)

@@ -98,19 +120,16 @@ def load_user(user_id):
@login_manager.request_loader
def load_user_request(request):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'request' from outer scope (line 1) (redefined-outer-name)

from .init import logger
from .url_converters import *

from functools import wraps
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W0611: Unused wraps imported from functools (unused-import)

@@ -112,6 +119,10 @@ def repo_card_view():
@app.route('/collection/status')
def status_view():
return render_module("status", title="Status")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)


@app.route('/connection_status')
def server_ping_frontend():
return render_module("ping")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)

@@ -318,6 +329,7 @@ def user_group_view(group = None):
return render_module("user-group-repos-table", title="Repos", repos=data, query_key=query, activePage=params["page"], pages=page_count, offset=pagination_offset, PS="user_group_view", reverse = rev, sorting = params.get("sort"), group=group)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)

backend_config = requestJson("config/get", False)
backend_config = AugurConfig(logger, db_session).load_config()

with get_session() as session:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'session' from outer scope (line 8) (redefined-outer-name)


logger = AugurLogger("augur").get_logger()

@app.route(f"/{AUGUR_API_VERSION}/admin/shutdown")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults to GET requests. Shouldn't this be POST only? Would prevent CSRF.

return jsonify({"status": "shutdown"})

@app.route(f"/{AUGUR_API_VERSION}/admin/restart")
@admin_required
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as admin/shudown. I think this should also be POST only.

@app.route(f"/{AUGUR_API_VERSION}/admin/restart")
@admin_required
def restart_system():
Popen("python scripts/control/restart.py", shell=True)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use absolute path derived from __file__ or the project root instead of relative path?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also is this script going to work for docker installs?


return response, 426

@app.route(f"/{AUGUR_API_VERSION}/config/get", methods=['GET', 'POST'])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we allowing POST here?


return jsonify(config_dict), 200

@app.route(f"/{AUGUR_API_VERSION}/config/set", methods=['GET', 'POST'])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above. Config mutations via GET are a CSRF risk and leak values into server logs, browser history, and referer headers. This should be POST-only with a JSON body.


return jsonify({"status": "success"})

@app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST'])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_config is missing @admin_required. The new set_config_item above it has admin protection, but this existing endpoint that can overwrite the entire config is unprotected. Should be:

Suggested change
@app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST'])
@app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST'])
@ssl_required
@admin_required
def update_config():

Comment on lines +161 to +169
def admin_required(func):
@login_required
@wraps(func)
def inner_function(*args, **kwargs):
if current_user.admin:
return func(*args, **kwargs)
else:
abort(403)
return inner_function
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The admin_required decorator has a subtle ordering issue. @login_required is applied to inner_function after @wraps(func), but login_required returns its own wrapper that doesn't preserve the metadata set by @wraps. This means the final returned function loses func's __name__ and __module__. Maybe we can use this approach:

Suggested change
def admin_required(func):
@login_required
@wraps(func)
def inner_function(*args, **kwargs):
if current_user.admin:
return func(*args, **kwargs)
else:
abort(403)
return inner_function
def admin_required(func):
@wraps(func)
def inner_function(*args, **kwargs):
if not current_user.is_authenticated:
return current_app.login_manager.unauthorized()
if current_user.admin:
return func(*args, **kwargs)
else:
abort(403)
return inner_function

errout.close()
except Exception as e:
logger.error(e)
raise e
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise e inside the 500 error handler will crash the handler itself, meaning the user never sees the friendly error page. Is that the intended idea?

backend_config = requestJson("config/get", False)
backend_config = AugurConfig(logger, db_session).load_config()

with get_session() as session:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session here shadows Flask's session import at the top of the file. Not a bug right now but could cause confusion. Consider db_session or db_sess instead?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IDs stats-section is reused on three different <h1> elements (Stats, User List, Config). Should be stats-section, user-section, and config-section to avoid issues down the line?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch() calls for restart/shutdown don't specify { method: 'POST' }. They default to GET. If the backend endpoints get fixed to POST-only (as suggested above), these need updating too:

fetch(url, { method: 'POST' }).then(response => { ... });

@MoralCode
Copy link
Copy Markdown
Collaborator

MoralCode commented Mar 4, 2026

@shlokgilda some context: i think the point of this PR is mainly to bring the admin page back (it used to be merged but somehow got unmerged)

definitely agree with a lot of the feedback though

im very hesitant about including "start and stop your instance" controls in our mixed manual/docker setup environment

Copy link
Copy Markdown
Collaborator

@sgoggins sgoggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ulincsys : Sorry I missed this earlier in the stack of PRs. Since all of this is really only code that Admin touches, or affects admin, and I don't see any of the general API changes as problematic, LGTM.

@MoralCode
Copy link
Copy Markdown
Collaborator

MoralCode commented Mar 16, 2026

As discussed in the March 10 maintainers meeting:

Most of these API endpoints, especially for restarting the Instance, assume a non-docker install. Thus this restarting only works on bare metal
This also relies on the jumpstart part if the code still existing (consider removing if exists as its only useful outside docker)

Many of these endpoints are likely disabled anyway and Augur’s core functionality has changed since this was merged

Id like to avoid merging endpoints that are disabled and dont have a future so we dont need a followup PR to remove them later

@sgoggins
Copy link
Copy Markdown
Collaborator

As discussed in the March 10 maintainers meeting:

Most of these API endpoints, especially for restarting the Instance, assume a non-docker install. Thus this restarting only works on bare metal This also relies on the jumpstart part if the code still existing (consider removing if exists as its only useful outside docker)

Many of these endpoints are likely disabled anyway and Augur’s core functionality has changed since this was merged

Id like to avoid merging endpoints that are disabled and dont have a future so we dont need a followup PR to remove them later

I will wait for @Ulincsys to address these points. Have we verified the restart does not work on Docker? I may have missed that discussion point at the last meeting.

@Ulincsys
Copy link
Copy Markdown
Collaborator Author

@sgoggins I'm planning to address the comments on this tonight after I get home.

We did discuss the restart endpoints during the last meeting. They make the assumption that they are not running in Docker, and so should are not suitable for attempting a restart from within Docker

@MoralCode
Copy link
Copy Markdown
Collaborator

I see potentially two forward paths:

  1. if jumpstart is still present in the code and we intend to keep it, then we can potentially set these routes up to only be present on the wbe server/in the admin UI when we are not running in docker.

  2. if jumpstart isnt present/these routes arent enabled anyway, we should remove them and their UI elements since they wont be functional and wont work.

It seems like the benefit of this UI was for admin convenience when augur was not yet dockerized and startup/shutdown was a special sequence of commands that needed remembering/referring to. with docker, it can be as simple as running docker compose stop from the same folder as augurs containerfile, or docker stop <one or more container names> to stop pieces of augur.

I worry that, if we have endpoints that allow instances to be restarted in the UI, that could present a much larger DoS attack surface, or general user inconvenience in multi-admin/multi-tenancy scenarios, than the convenience it provides.

I can see a benefit to controlling augur tasks within the admin dashboard, but im not sure we need shutdown and restart endpoints

@MoralCode
Copy link
Copy Markdown
Collaborator

Jumpstart does appear to still be in the repo. last touched 2 years ago.

Im very tempted to exclude it from the docker builds. maybe we can have the code check if jumpstart is available before trying to use it and make these endpoints no-ops on docker?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants